feat: Update nes/gbc/sms/genesis emulators to use shared memory #99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add `shared_memory` component for sharing memory between the emulators more easily * Refactored all emulators to use new shared memory for majority of state and data. This means each emulator component now has less than 5k of D/IRAM in use that is not from shared memory. * Update to have better clear screen function that wont have any race conditions and wont destroy buffers. Update video task to have simple clear screen function if passed nullptr. * Update cmake lists to use newer version of cmake * Update to hid roms from rom list if their emulator is not enabled for easier / faster iteration and testing when adding / modifying emulators * Update espp to latest Shared memory means that it is not easier to add more emulators (such as msx and doom) since the emulators no longer fill up the internal ram. This does require in some cases modifying the emulators to leverage the shared memory, but this is a one-time operation and actually helps ensure the emulator maintains high performance by keeping its state within internal memory as much as possible, instead of in offboard PSRAM. Build and run `main` on box-3 and ensure all emulators work for multiple sessions of different roms in sequence. Also ensure that saving state and loading state works for each emulator.
finger563
added
enhancement
Contributor
There was a problem hiding this comment.
Copilot reviewed 80 out of 84 changed files in this pull request and generated 2 comments.
Files not reviewed (4)
- CMakeLists.txt: Language not supported
- components/espp: Language not supported
- components/gbc/CMakeLists.txt: Language not supported
- components/genesis/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (1)
components/box-emu/src/box-emu.cpp:736
- [nitpick] The arithmetic used to select between vram0 and vram1 is unclear. Consider using a ternary operator to improve readability.
Pixel* _buf = (Pixel*)((uint32_t)vram0 * (vram_index ^ 0x01) + (uint32_t)vram1 * vram_index);
Comment on lines
+64
to
+65
| scan = (struct gbc_scan*)shared_mem_allocate(&gbc_scan_request); | ||
|
|
There was a problem hiding this comment.
The shared memory allocation for 'scan' is not checked for a null return value. Consider adding a check to handle allocation failures.
Suggested change
| scan = (struct gbc_scan*)shared_mem_allocate(&gbc_scan_request); | |
| scan = (struct gbc_scan*)shared_mem_allocate(&gbc_scan_request); | |
| if (!scan) { | |
| ESP_LOGE(TAG, "Failed to allocate GBC scan object"); | |
| return; | |
| } |
Comment on lines
+78
to
+79
| cpu = (struct cpu*)shared_mem_allocate(&cpu_request); | ||
|
|
There was a problem hiding this comment.
There is no null check after allocating shared memory for 'cpu'. Adding error handling here can prevent potential null pointer dereferences.
Suggested change
| cpu = (struct cpu*)shared_mem_allocate(&cpu_request); | |
| cpu = (struct cpu*)shared_mem_allocate(&cpu_request); | |
| if (!cpu) { | |
| ESP_LOGE(TAG, "Failed to allocate CPU structure"); | |
| return; | |
| } |
|
⚡ Static analysis result ⚡ 🔴 cppcheck found 1 issue! Click here to see details.esp-box-emu/components/shared_memory/src/shared_memory.c Lines 64 to 69 in 36acf90 !Line: 64 - portability: %d in format string (no. 1) requires 'int' but the argument type is 'size_t {aka unsigned long}'. [invalidPrintfArgType_sint]
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
shared_memorycomponent for sharing memory between the emulators more easilyMotivation and Context
Shared memory means that it is not easier to add more emulators (such as msx and doom) since the emulators no longer fill up the internal ram. This does require in some cases modifying the emulators to leverage the shared memory, but this is a one-time operation and actually helps ensure the emulator maintains high performance by keeping its state within internal memory as much as possible, instead of in offboard PSRAM.
How has this been tested?
Build and run
mainon box-3 and ensure all emulators work for multiple sessions of different roms in sequence. Also ensure that saving state and loading state works for each emulator.Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action.